Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MVP of Accessibility/UI automation support #5177

Merged
merged 86 commits into from
Mar 2, 2022
Merged

Conversation

grokys
Copy link
Member

@grokys grokys commented Dec 14, 2020

What does the pull request do?

Adds a Minimum Viable Implementation of Accessibility/UI automation to Avalonia along with the start of some integration tests using Appium.

There is still a lot missing here, but the PR is already huge so I'd like to propose getting what's here merged and improving it in smaller PRs.

The API tries to stick as closely to the WPF/UWP API as possible, while adding a few improvements.

Platforms

  • Windows (UIA)
  • MacOS (NSAccessibility)
  • Linux (AT-SPI2)
    • Can't get Tmds.DBus to work with this

Providers

  • IDockProvider
  • IExpandCollapseProvider
  • IGridItemProvider
  • IGridProvider
  • IInvokeProvider
  • IItemContainerProvider
  • IMultipleViewProvider
  • IRangeValueProvider
  • IRootProvider (IWindowProvider in WPF)
  • IScrollItemProvider
  • IScrollProvider
  • ISelectionItemProvider
  • ISelectionProvider
  • ISynchronizedInputProvider
  • ITableItemProvider
  • ITableProvider
  • ITextProvider/ITextRangeProvider
    • I have an implementation of this, but the Windows model is so different from the MacOS model for text accessibility that it will need some more work
  • IToggleProvider
  • ITransformProvider
  • IValueProvider
  • IVirtualizedItemProvider

Controls

  • Calendar
  • CaptionButton
  • TitleBar
  • DatePicker
  • TimePicker
  • MenuFlyout
  • Notification
  • NumericUpDown
  • ScrollBar
  • TabStrip
  • ItemsRepeater
  • Button
  • ButtonSpinner
  • Carousel
  • CheckBox
  • ComboBox
  • ContextMenu
  • Expander
  • Image
  • ItemsControl
  • Label
  • ListBox
  • MaskedTextBox
  • Menu
  • ProgressBar
  • RadioButton
  • RepeatButton
  • ScrollViewer
  • Slider
  • Spinner
  • SplitView
  • TabControl
  • TextBlock
  • ToggleSwitch
  • TreeView
  • Window
  • DataGrid

Integration Tests

  • Button
  • CheckBox
  • ComboBox
  • ListBox
  • Menu
  • NativeMenu

Integration tests are currently using Appium in order to only need to write one set for Windows/MacOS. However Appium is very buggy on Windows/MacOS and also quite limited:

  • WinAppDriver is currently unmaintained and has problems doing basic stuff like Ctrl+Click
  • appium-mac2-driver is maintained but in an even worse state - it too can't mange things like Ctrl+Click, or even closing windows without crashing
  • There doesn't seem to be any support for listening to a11y events in the C# client

Maybe that we need to write platform-specific integration tests in the long run, but for now lets see how far Appium can take us.

Fixed issues

Fixes #585

@grokys
Copy link
Member Author

grokys commented Dec 14, 2020

@jkoritzinsky in #585 it sounded like you had some ideas for this, and sounds like it's not really along the lines of what you were thinking (instead it's loosely based on WPF's automation system). Is this the right direction do you think?

@MarchingCube
Copy link
Collaborator

To sum up my initial testing - I was able to use https://github.com/microsoft/WinAppDriver to interact with ControlCatalog running on top of this branch. I was able to navigate between tab pages and click buttons. I was mostly using names and type names to find things. I can see that having support for automation id will be useful: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.automationproperties.automationidproperty?view=winrt-19041

@grokys
Copy link
Member Author

grokys commented Feb 22, 2021

@MarchingCube
Copy link
Collaborator

@grokys Right, I didn't realize that it is an attached property. I will try testing again sometime soon.

@grokys grokys force-pushed the feature/ui-automation branch 2 times, most recently from 5bef7e3 to 2dbccc0 Compare March 1, 2021 08:58
@grokys
Copy link
Member Author

grokys commented Mar 5, 2021

As I keep working on this the more I hate the WPF/UWP AutomationPeer API... You end up having to build three trees: Logical Tree -> AutomationPeer tree -> OS Automation Node Tree and keep them synchronized. For what end?

The more I look at the OSX NSAccessibility API the more I feel jealous. Compare the workflow in WPF and OSX to make a control an accessible button.

WPF:

  • Create a MyButtonAutomationPeer
  • Implement the IInvokeProvider interface on the peer
  • Override OnCreateAutomationPeer on MyButton to return a MyButtonAutomationPeer

OSX:

Does anyone have any experience of these APIs? Is there an advantage to the WPF/UWP way of doing it?

Follows WPF/UWP API as closely as possible. Limited to win32 right now. Broken in many places.
@grokys grokys force-pushed the feature/ui-automation branch from 17887bb to 2a44d8b Compare March 9, 2021 12:26
@grokys
Copy link
Member Author

grokys commented Mar 9, 2021

Ok, due to my inexperience with these APIs and the general discussion over at #585 I've decided to start by modelling the WPF API as closely as possible. From there we'll have to adjust it as we implement support for other platforms.

Copy link

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the UIA API from WPF/UWP are good and I think the API you have so far (besides my remarks/comments) is good. One thing you could change is that the AutomationControlType could also be set from XAML. Some peers only seem to exist to override this peer, this might not be ideal. In contrast, the Web A11y API (aria) only works using HTML properties, so it might be worth exploring moving things that don't need to be done from code behind into XAML (e.g. AutomationControlType).


namespace Avalonia.Automation.Peers
{
public enum AutomationControlType

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would double check this list with the ARIA roles as those is a minimum supported on most platforms: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this list comes from the WPF API. Should we just replace this list with values from the ARIA roles if that's the common subset? Should we keep the WPF/win32 naming or use the ARIA naming? Perhaps that latter would make more sense from an x-plat point of view.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sticking with the WPF/Win23/UWP naming is easier since folks familiar with XAML are more familiar with those names. Developers coming from the Web will have to learn XAML first anyway, having some web naming in here might seem weird then.

Node.PropertyChanged(automationProperty, oldValue, newValue);
}

protected virtual string GetLocalizedControlTypeCore()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not being localized intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not done yet. Would it be best to use satellite assembles for localizing the names, or just hardcode the names for all languages in code/resources in the main assembly?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably hardcoding is fine? Not sure how localization works with Avalonia so whatever is the best way of doing it in Avalonia might be best here. But as a developer I would expect this assembly to be able to return translated control types.


namespace Avalonia.Automation
{
public static class AutomationProperties

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A HeadingLevel property is missing, however it makes scanning documents a lot easier for people using assistive technology. Also, a LandMarkType property is missing but is very important for people with assistive technology.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was ported from WPF which doesn't have HeadingLevel/LandmarkType . I can add those.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those properties are present in UWP and the web (as semantic html). Honestly, I'm a bit surprised that WPF didn't have those.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we add the HeadingLevel or LandMarkType properties? When using narrator on Windows, those are definitely helping people navigate UWP/WinUI apps more easily and those properties are also present in web accessibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely, as I mention in the PR description though, this PR is already huge so stuff like this will be implemented later.


protected override AutomationControlType GetAutomationControlTypeCore()
{
return AutomationControlType.Group;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not sound right, "None" content is a group? How about just removing it from UIA tree then instead of saying it is a group?

Copy link
Member Author

@grokys grokys May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with completely removing them from the OS-level automation tree is that it breaks the AutomationPeer -> OS automation node 1:1 mapping and means that we have to have another level of abstraction to handle parent/child relationships in the case where peers don't map directly to OS automation nodes. We've already got 3 trees to keep in sync so at least if the nodes in the "peer" tree match 1:1 with the nodes in the OS tree it reduces some complexity.

Win32 and OSX provide a facility for "hiding" controls at the automation node level (UIA_IsControlElementPropertyId/accessibilityElement), which looked to me to be sufficient?

Maybe instead of using Group we should have a new AutomationControlType.None type which will be mapped to the most relevant type for "ignored" elements at the OS level?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. In that case, group is fine, having a "none" control type would be really cool, however I'm not sure how much of a benefit there would be.

@@ -17,6 +18,7 @@ public partial class WindowImpl
protected virtual unsafe IntPtr AppWndProc(IntPtr hWnd, uint msg, IntPtr wParam, IntPtr lParam)
{
const double wheelDelta = 120.0;
const long UiaRootObjectId = -25;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the -25 arbitrary here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, good to know. Could add a comment for this, but I think it's fine as is.

@grokys
Copy link
Member Author

grokys commented May 31, 2021

Thanks so much for the review @chingucoding - as I've mentioned I'm new to this stuff so getting some input from someone with experience in this area is invaluable.

One thing you could change is that the AutomationControlType could also be set from XAML. Some peers only seem to exist to override this peer, this might not be ideal.

Sounds like a good idea - so something like <Button AutomationProperties.AutomationControlType="foo"> which when set overrides the control type in the peer?

In contrast, the Web A11y API (aria) only works using HTML properties, so it might be worth exploring moving things that don't need to be done from code behind into XAML (e.g. AutomationControlType).

Yep, peers definitely do seem heavyweight from my point of view compared to HTML area properties! Any other ideas for things we could do directly from XAML?

@grokys
Copy link
Member Author

grokys commented May 31, 2021

In addition, I have an initial implementation for OSX that I need to merge into this PR. Are you familiar with a11y on OSX at all?

Previous change showed up a problem where incorrect interface was getting called for `IsReadOnly`.
Copy link

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is heading in the right direction. There are still thinks that need to be implement but that is expected since this is the MVP. In my opinion, this would be good API wise (didn't check all implementations exactly).

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0017726-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@grokys grokys force-pushed the feature/ui-automation branch from 51b3f5b to bff4616 Compare January 21, 2022 12:57
@Takoooooo Takoooooo added this to the 11.0 milestone Feb 21, 2022
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018922-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

1 similar comment
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018922-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018944-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

1 similar comment
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018944-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

Copy link
Member

@danwalmsley danwalmsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm yolo

@grokys grokys enabled auto-merge March 2, 2022 13:31
@grokys grokys merged commit 6b7ea2f into master Mar 2, 2022
@grokys grokys deleted the feature/ui-automation branch March 2, 2022 13:49
@webczat
Copy link

webczat commented Mar 2, 2022

is usage of ATK instead of AT-SPI a viable option for adding linux accessibility? might be easier... or not

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018988-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

1 similar comment
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018988-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@robloo
Copy link
Contributor

robloo commented Mar 2, 2022

Great PR! and I know it's already merged. Just wanted to voice my concern about putting all automation peers in a directory separate from the control implementations. src/Avalonia.Controls/Automation/Peers/ComboBoxAutomationPeer.cs. In my opinion (and following standard practices) these should go with the control implementations directly. See #4957

zijwij-byxmyq-1wIxre

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility support for targeted platforms needs to be considered